feat: add support for Sass pkg: importers
#384
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Matches the RequestService passed in from the language server extension.
wkillerud
commented
Feb 16, 2024
src/cssLanguageTypes.ts
Outdated
| @@ -284,6 +284,7 @@ export interface FileStat { | |||
| export interface FileSystemProvider { | |||
| stat(uri: DocumentUri): Promise<FileStat>; | |||
| readDirectory?(uri: DocumentUri): Promise<[string, FileType][]>; | |||
| getContent?(uri: DocumentUri, encoding?: BufferEncoding): Promise<string>; | |||
There was a problem hiding this comment.
Matches the RequestService on the language server side
This was referenced Feb 16, 2024
aeschli
requested changes
Feb 28, 2024
src/services/cssNavigation.ts
Outdated
| // look for the `exports` field of the module and any `sass`, `style` or `default` that matches the import. | ||
| // If it's only `pkg:module`, also look for `sass` and `style` on the root of package.json. | ||
| if (target.startsWith('pkg:')) { | ||
| const bareTarget = target.replace('pkg:', ''); |
There was a problem hiding this comment.
can this be extracted in a method ?
src/services/cssNavigation.ts
Outdated
| try { | ||
| return await this.fileSystemProvider.getContent(uri); | ||
| } catch (err) { | ||
| console.error(err); |
There was a problem hiding this comment.
Should not print to console
Asmodeusxxx85
approved these changes
Mar 27, 2024
aeschli
approved these changes
Jun 24, 2024
|
Thanks @wkillerud. I made some improvements and moved the code to |
chrmarti
approved these changes
Jun 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #383
Hello 👋 I'm opening a PR with a suggested solution for #383
As mentioned in the issue, Sass released a new syntax for import strings, which I imagine trips up a lot of tools. I find the documentation for NodePackageImporter has the most readable explanation of the different supported scenarios. Also relevant here is the Node documentation on subpath patterns.
It's a big chunk with many different code paths, I'll admit. I've tried documenting at a reasonable level, and added tests that should cover a lot of scenarios. That said, I understand if you're hesitant to add yet more module resolution logic here.
I've tested locally with
npm linkin a running extension using these test fixtures, and the resolution provides correct links for me. Recording of that below.CleanShot.2024-02-16.at.21.51.49.mp4
Considered alternatives
Sass's JavaScript API includes the aforementioned NodePackageImporter. Its implementation seems to have a function
canonicalize, which looked promising as a method of looking up an import string. Unfortunately it's not part of the public API.The result from
sass.compileincludes all the loaded URLs, so I'm sure you could hack something together there (pass in a string that only@uses thepkg:moduleimport in question). It would require bundlingsassthough, which is also technically possible now.